fix(web): default RunJob.Delete to true for API-created and persisted jobs - #745
Conversation
… jobs
RunJob.Delete only receives its "true" default (core/runjob.go) through
the config.ini decoder's default-tag handling. Jobs built directly from
a jobRequest (web/server.go newRunJobFromRequest) or restored from the
state file on daemon restart (cli/daemon.go buildPersistedRunJob) never
go through that decoder, so Delete was left at its zero value (""), and
deleteContainer() treats that as false. Every job-run created via the
API left its container behind, colliding with the next scheduled run's
`docker create` on the same name ("resource conflict").
jobRequest has no "delete" field to work around this at the call site,
and the two construction paths are independent of each other, so both
needed the same one-line fix to match the config.ini default behavior.
Signed-off-by: Matheus Amendola <matheusamendolaa@gmail.com>
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Pull request overview
This PR fixes a lifecycle bug in Ofelia’s type=run jobs created via the web API and restored from the daemon state file: core.RunJob.Delete was left at its zero value ("") outside the INI decoder path, causing finished containers to be retained and subsequent runs to fail with Docker “resource conflict” errors.
Changes:
- Default
RunJob.Deleteto"true"when constructing run jobs from API requests (web/server.go). - Default
RunJob.Deleteto"true"when reconstructing persisted run jobs on daemon restart (cli/daemon.go). - Add regression tests covering both construction paths.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
web/server.go |
Sets RunJob.Delete = "true" for API-created run jobs to ensure containers are removed after completion. |
web/server_run_job_delete_default_test.go |
Adds a regression test asserting API-created *core.RunJob defaults Delete to "true". |
cli/daemon.go |
Sets RunJob.Delete = "true" when rebuilding persisted run jobs after restart. |
cli/daemon_persisted_run_job_delete_default_test.go |
Adds a regression test asserting persisted *core.RunJob defaults Delete to "true". |
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #745 +/- ##
==========================================
+ Coverage 87.45% 87.57% +0.11%
==========================================
Files 90 90
Lines 12010 12012 +2
==========================================
+ Hits 10503 10519 +16
+ Misses 1218 1204 -14
Partials 289 289
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
CybotTM
left a comment
CybotTM
left a comment
There was a problem hiding this comment.
Thanks for tracking this down — great find, and the reproduction plus root-cause trace in the description made this easy to verify. The diagnosis is correct (Delete gets its "true" default only via the config decoder's default tag, and strconv.ParseBool("") → false in deleteContainer), both construction paths are covered, and the regression tests pin exactly the right thing. Also confirmed persist.Job has no delete field, so forcing "true" on restore can't override an explicit user choice.
One change requested before merge: instead of hand-setting j.Delete = "true", please use the codebase's existing convention — the config path applies all struct-tag defaults via creasty/defaults (_ = defaults.Set(j) in registerAllJobs, cli/config.go). Calling _ = defaults.Set(j) in newRunJobFromRequest and buildPersistedRunJob instead:
- closes the sibling gaps of the same bug class in the same stroke:
Pull(default:"true"— API/persisted jobs currently run withpull=false, so a locally-present tag is never refreshed) andHistoryLimit(default:"10"— currently0, so run history is never trimmed and grows unbounded); - is future-proof: if a
deletefield is ever added tojobRequest/persist.Job, an explicit"false"survives while unset still defaults to"true"(defaults.Setonly fills zero values); - needs no new dependency —
creasty/defaultsis already ingo.mod;webjust adds the import.
Your regression tests should pass unchanged and then guard the broader fix; the explanatory comments at the two call sites can shrink to a line each ("struct-tag defaults are only applied by the config decoder — apply them here too").
Per review on netresearch#745: reuse the codebase's existing struct-tag-default convention (creasty/defaults, already used in registerAllJobs) instead of hand-setting Delete = "true". This also fixes the sibling gaps of the same bug class for API-created and persisted run jobs: Pull (default:"true", was silently false) and HistoryLimit (default:"10", was 0, unbounded run history). Future-proof: defaults.Set only fills zero values, so an explicit false survives if delete is ever exposed in jobRequest/persist.Job. Signed-off-by: Matheus Amendola <matheusamendolaa@gmail.com>
|
Applied — both call sites now use j := core.NewRunJob(s.provider)
// struct-tag defaults are only applied by the config decoder — apply them here too.
_ = defaults.Set(j)
j.Name = req.Name
...Confirmed |
|
Summary
RunJob.Delete(core/runjob.go) only gets its"true"default through the config.ini decoder'sdefaultstruct tag. Jobs built directly from ajobRequest(newRunJobFromRequest,web/server.go) or rebuilt from the state file on daemon restart (buildPersistedRunJob,cli/daemon.go) never go through that decoder, soDeleteis left at its zero value ("").deleteContainer()(core/runjob.go) treats that asfalseviastrconv.ParseBool(""), so the container from everytype=runjob created through the web API is left behind after it finishes.On the next scheduled run,
docker createfails because a container with that name already exists:jobRequestdoesn't expose adeletefield to work around this at the call site, and the two construction paths (live API creation vs. state-file restore on restart) are independent, so both needed the same one-line fix to match the behavior config.ini users already get by default.Found and reproduced against a real deployment (a daily ETL job created through the web UI started failing every day once its previous container was left behind — confirmed with
docker ps -ashowing theExited (0)container days after its last run, and by reading the source to trace why).Changes
web/server.go:newRunJobFromRequestnow setsj.Delete = "true".cli/daemon.go:buildPersistedRunJobnow setsrj.Delete = "true".*core.RunJobhasDelete == "true".Test plan
go build ./...go vet ./...go test ./web/... ./cli/...(full package suites, not just the new tests)gofmt -l .cleantype=runjob viaPOST /api/jobs/create, ran it twice in a row (second run previously failed withresource conflict, now succeeds and container is removed both times); also verified across a daemon restart (buildPersistedRunJobpath) withOFELIA_STATE_FILEset — same result.